Improve Android haptic, offer toggle for haptics in the app #3482
Conversation
|
The Pull Request introduced fingerprint changes against the base commit: 9007810 Fingerprint diff[{"type":"dir","filePath":"node_modules/expo-haptics/android","reasons":["expoAutolinkingAndroid"],"hash":"61e8ca125d3dce6a811261f44acf7f1833dbbb3b"},{"type":"dir","filePath":"patches","reasons":["patchPackage"],"hash":"9d940021d8864bd3ca6f3afb366f26b2d07e25bc"}]Generated by PR labeler 🤖 |
|
haileyok
force-pushed
the
hailey/haptics-config
branch
from
ee3f809 to
7b413d0
Compare
src/lib/haptics.ts
Outdated
| impactAsync(hapticImpact) | ||
| } | ||
| static impact(type: ImpactFeedbackStyle = hapticImpact) { | ||
| if (isWeb) { | ||
| static impact(type: ImpactFeedbackStyle = hapticImpact, disabled: boolean) { |
There was a problem hiding this comment.
Where are these being used? I can't find callsites. Are these here just in case to match the Expo API? (Asking these about all methods other than default)
There was a problem hiding this comment.
I assume just to match, yea. That's how it was set up prior so I maintained the API but I'm fine with removing these.
There was a problem hiding this comment.
Simplified this down to just playHaptic(disabled: boolean). I don't think we have any intentions of using these other options, those originated from a while back and it does not look like they were ever used from what I can tell.
src/lib/haptics.ts
Outdated
| if (isWeb) { | ||
| static notification = ( | ||
| type: 'success' | 'warning' | 'error', | ||
| enabled: boolean, |
There was a problem hiding this comment.
Why do we have enabled here but disabled for others?
There was a problem hiding this comment.
forgot to invert this one, but removed anyway since we don't use it.
src/view/screens/ProfileFeed.tsx
Outdated
| removeFeed, | ||
| resetSaveFeed, | ||
| feedInfo.uri, |
There was a problem hiding this comment.
Unintended change? I suppose it doesn't matter but I'm not sure
There was a problem hiding this comment.
Linter probably picked them up when I added to the deps. Is probably fine, I'm fine with reverting the lint changes though if we want.
There was a problem hiding this comment.
Same to the others below.
src/view/screens/ProfileFeed.tsx
Outdated
| isHapticsDisabled, | ||
| isPinned, | ||
| unpinFeed, | ||
| feedInfo.uri, |
There was a problem hiding this comment.
Same: unintended or intentional
src/view/screens/ProfileFeed.tsx
Outdated
| unlikeFeed, | ||
| track, | ||
| likeFeed, | ||
| feedInfo.uri, |
* origin/main: (455 commits) Use getSuggestions endpoint behind the gate (#3499) Added `new_profile_scroll_component` to `Gate` type (#3487) Fix useGate lint rule (#3486) Make bio area scrollable on iOS (#2931) Improve Android haptic, offer toggle for haptics in the app (#3482) Search - only enable queries once tab is active (#3471) [Statsig] Mark Testflight as staging tier (#3470) [Statsig] Typecheck gates (#3467) Bump to 1.77 (#3468) Search - extra tabs (#3408) notify slack on production builds (#3461) notify slack on production builds (#3460) 1.76 release preparations (#3459) Update zh-CN translation (#3433) Italian Localization (#3388) [Statsig] Send prev route name (#3456) [Statsig] Instrument feed display (#3455) Small logic cleanups (#3449) Use ALF for the embed consent modal (#3336) Onboarding tweaks (#3447) ...
Fixes #3484
Why
Android Patch
We recently migrated to
expo-haptics, mostly in preparation for using Fabric. There's a difference however in how the two libraries run an iOS "light" haptic config on Android.The previous library used the
VibrationAPI solely, which does nothave any configuration for intensity of vibration. The
VibrationAPI has also been deprecated since SDK 26. See:https://github.com/mkuczera/react-native-haptic-feedback/blob/master/android/src/main/java/com/mkuczera/vibrateFactory/VibrateWithDuration.java
Expo Haptics is using
VibrationManagerAPI on SDK >= 31. See: https://github.com/expo/expo/blob/main/packages/expo-haptics/android/src/main/java/expo/modules/haptics/HapticsModule.kt#L44The timing and intensity of their haptic configurations though differs greatly from the original implementation. This
patch uses the new
VibrationManagerAPI to create the same vibration that would have been seen in the deprecatedVibrationAPI.We could just revert this change if we'd rather not use this patch. However, given the deprecated API and the preference I have in using Expo-supported modules as we move into the Fabric migration, I would suggest this change. Given that the haptics were actually not even present at all on Android (many people say that the haptics are actually NEW, not just changed), that feels like more reason to move away from the deprecated API.
Toggle
This issue has actually re-raised a lot of people's complaints about haptics in general. Even some iOS users would rather be able to toggle these off. It also isn't as common a behavior on Android, so some users there want to turn them off anyway. To that end, I've added a haptic toggle preference. The default is still to use haptics, but the user can disable them in the settings screen.
Test Plan
To be honest, I can not tell much of a difference between the two haptics on Android. This is probably because of varying haptic generators in different devices. Regardless, this change is relatively straight forward so I am not worried about that.
Also I have tested the various places that we have haptics in the app, and verified that the toggle does indeed disable haptics when turned on.